Skip to content

AdministratorName parameter should take value which is defined in enum #7377

Merged
tjprescott merged 9 commits intoAzure:masterfrom
SanjaMalesevic:changeJsonForADAdminoOnMI
Nov 7, 2019
Merged

AdministratorName parameter should take value which is defined in enum #7377
tjprescott merged 9 commits intoAzure:masterfrom
SanjaMalesevic:changeJsonForADAdminoOnMI

Conversation

@SanjaMalesevic
Copy link
Copy Markdown
Contributor

@SanjaMalesevic SanjaMalesevic commented Oct 1, 2019

No description provided.

@openapi-sdkautomation
Copy link
Copy Markdown

openapi-sdkautomation bot commented Oct 1, 2019 •
edited
Loading

In Testing, Please Ignore

[Logs] (Generated from 7555722, Iteration 3)

Warning .NET: test-repo-billy/azure-sdk-for-net [Logs] [Diff]
  • No packages generated.
Succeeded Python: test-repo-billy/azure-sdk-for-python [Logs] [Diff]
Failed Java: test-repo-billy/azure-sdk-for-java [Logs] [Diff]
Succeeded Go: test-repo-billy/azure-sdk-for-go [Logs] [Diff]
Failed JavaScript: test-repo-billy/azure-sdk-for-js [Logs] [Diff]
Succeeded Ruby: test-repo-billy/azure-sdk-for-ruby [Logs] [Diff]

@azuresdkci azuresdkci requested a review from tjprescott October 1, 2019 18:07
@azuresdkci
Copy link
Copy Markdown
Contributor

azuresdkci commented Oct 1, 2019

Can one of the admins verify this patch?

@SanjaMalesevic
Merge pull request #1 from Azure/master
e0829d2 
Merge changes from master
@AutorestCI AutorestCI mentioned this pull request Oct 1, 2019
Closed
@AutorestCI
Copy link
Copy Markdown

AutorestCI commented Oct 1, 2019 •
edited
Loading

Automation for azure-sdk-for-python

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-python#8425

@AutorestCI AutorestCI mentioned this pull request Oct 1, 2019
Closed
@AutorestCI
Copy link
Copy Markdown

AutorestCI commented Oct 1, 2019 •
edited
Loading

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#6294

@openapi-sdkautomation openapi-sdkautomation bot mentioned this pull request Oct 1, 2019
Closed
@SanjaMalesevic SanjaMalesevic changed the title Change AdministratorName parameter, it should take constant value defined in enum AdministratorName parameter should take value which is defined in enum Oct 2, 2019
@tjprescott tjprescott added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Oct 23, 2019
tjprescott
tjprescott reviewed Oct 23, 2019
View reviewed changes
...resource-manager/Microsoft.Sql/preview/2017-03-01-preview/managedInstanceAdministrators.json Outdated
"type": "string"
"type": "string",
"enum": [
"ActiveDirectory"
Copy link
Copy Markdown
Member

@tjprescott tjprescott Oct 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the change from string to enum result in a breaking change for some SDKs?

Copy link
Copy Markdown
Contributor Author

@SanjaMalesevic SanjaMalesevic Oct 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, there is a difference in number of parameters for create/modify/delete methods in .NET and Python clients between MI and SQL DB. So, this change is made to aligned MI clients with SQL DB (they need to be auto-generated again based on updated json).
Jared has noticed in PR Azure/azure-cli#10446 that there is parameter AdministratorName in method create_or_update which always takes the same value and that this parameter doesn't exist in clients for sql db.
I am not sure, but I think that this is a breaking change.

Copy link
Copy Markdown
Contributor

@jaredmoo jaredmoo Oct 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is a breaking change. However this is a new API and therefore right now is the time that break will have minimal impact. Also this change brings managed instance admin API into sync with server admin API.

@SanjaMalesevic
Copy link
Copy Markdown
Contributor Author

SanjaMalesevic commented Nov 4, 2019

Hi all, this is a gentle reminder to take look at this PR.
Thanks,
Sanja

@tjprescott
Copy link
Copy Markdown
Member

tjprescott commented Nov 4, 2019

@Azure/arm-api-review-board please take a look.

majastrz
majastrz requested changes Nov 4, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a comment. Please take a look.

@majastrz
Copy link
Copy Markdown
Member

majastrz commented Nov 4, 2019

Some of the CI/CD checks are failing on this PR also. Please take a look.

This was referenced Nov 6, 2019
Closed
Merged
@SanjaMalesevic
Copy link
Copy Markdown
Contributor Author

SanjaMalesevic commented Nov 6, 2019

Some of the CI/CD checks are failing on this PR also. Please take a look.

I saw from the error log that ModelValidation failed when trying to validate "examples" and "x-ms-examples", but there is no specific error here. @majastrz could you please help me to figure out what is wrong here?

@majastrz
Copy link
Copy Markdown
Member

majastrz commented Nov 6, 2019 •
edited
Loading

If you keep clicking links on the Details next to the failing check, you will eventually get to the Model Validation errors.

I copied the following from https://dev.azure.com/azure-sdk/public/_build/results?buildId=181455&view=logs&j=8d2f9c49-cd83-5a9d-f3fe-2fd651afa742&t=43324f1f-6724-5d61-785f-f0ac9fd256a1:

 error : 
operationId: ManagedInstanceAdministrators_CreateOrUpdate
scenario: Update administrator of managed instance
source: response
responseCode: '200'
severity: 0
code: OBJECT_ADDITIONAL_PROPERTIES
details:
  code: OBJECT_ADDITIONAL_PROPERTIES
  params:
    - location
  message: 'Additional properties not allowed: location'
  path: ''
  title: '#/definitions/ManagedInstanceAdministrator'
  description: An Azure SQL managed instance administrator.
  position:
    line: 325
    column: 37
  url: >-
    specification/sql/resource-manager/Microsoft.Sql/preview/2017-03-01-preview/managedInstanceAdministrators.json
  directives: {}
  jsonPosition:
    line: 19
    column: 15
  jsonUrl: >-
    specification/sql/resource-manager/Microsoft.Sql/preview/2017-03-01-preview/examples/ManagedInstanceAdministratorUpdate.json
  jsonPath: ''

 error : 
operationId: ManagedInstanceAdministrators_CreateOrUpdate
scenario: Update administrator of managed instance
source: response
responseCode: '201'
severity: 0
code: OBJECT_ADDITIONAL_PROPERTIES
details:
  code: OBJECT_ADDITIONAL_PROPERTIES
  params:
    - location
  message: 'Additional properties not allowed: location'
  path: ''
  title: '#/definitions/ManagedInstanceAdministrator'
  description: An Azure SQL managed instance administrator.
  position:
    line: 325
    column: 37
  url: >-
    specification/sql/resource-manager/Microsoft.Sql/preview/2017-03-01-preview/managedInstanceAdministrators.json
  directives: {}
  jsonPosition:
    line: 33
    column: 15
  jsonUrl: >-
    specification/sql/resource-manager/Microsoft.Sql/preview/2017-03-01-preview/examples/ManagedInstanceAdministratorUpdate.json
  jsonPath: ''

##[error]Bash exited with code '1'.
Finishing: Model Validation

It appears that there's an issue with the examples referencing a property that is not declared in the schema.

@majastrz majastrz added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed ARMChangesRequested WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Nov 6, 2019
majastrz
majastrz approved these changes Nov 6, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Signed off from ARM side.

@tjprescott
Copy link
Copy Markdown
Member

tjprescott commented Nov 6, 2019

/azurepipelines run

1 similar comment
@tjprescott
Copy link
Copy Markdown
Member

tjprescott commented Nov 6, 2019

/azurepipelines run

@tjprescott
Copy link
Copy Markdown
Member

tjprescott commented Nov 6, 2019

Please check on the build failure: https://dev.azure.com/azure-sdk/public/_build/results?buildId=181455&view=logs&j=d2a54e27-891b-5b91-4b1b-0c6d3d820755&t=4db5f656-3b59-5661-54ad-d4645c211b0f

@SanjaMalesevic
Copy link
Copy Markdown
Contributor Author

SanjaMalesevic commented Nov 7, 2019

I've fixed the examples and all checks have passed. Thanks for help.

@tjprescott tjprescott merged commit 5e95a31 into Azure:master Nov 7, 2019
@SanjaMalesevic SanjaMalesevic mentioned this pull request Nov 8, 2019
Merged
solhaile pushed a commit to solhaile/azure-rest-api-specs that referenced this pull request Nov 13, 2019
@SanjaMalesevic
AdministratorName parameter should take value which is defined in enum (
12ac17e 
Azure#7377)

* AdministratorName parameter should take constant value defined in enum

* Code review fixes

* Code review fixes

* Fix tabs and spaces

* Fix example with get

* revert example with get

* Fix examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tjprescott tjprescott tjprescott approved these changes

@majastrz majastrz majastrz approved these changes

+1 more reviewer

@jaredmoo jaredmoo jaredmoo approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@tjprescott tjprescott

Labels

ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@SanjaMalesevic @azuresdkci @AutorestCI @tjprescott @majastrz @jaredmoo